-
Notifications
You must be signed in to change notification settings - Fork 358
Added custom fetchOptions management for overlays, added a proxyUrl parameter #1336
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
…ed a proxy url and fetchOptions parameter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! I think we can also remove this line that assigns the tiles renderer fetch options to the "ImageSource" since these changes are ensuring that the servers used for tile geometry and tile images are treated separately.
| fetch( url, options = {} ) { | ||
|
|
||
| return fetch( ...args ); | ||
| const finalUrl = this.proxyUrl ? `${ this.proxyUrl }${ encodeURIComponent( url ) }` : url; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of "proxyURL" lets add an optional "preprocessURL" callback function similar to the plugins that can transform the url. Then users can format the url however they need. Something like so:
if ( this.preprocessURL ) {
url = this.preprocessURL( url );
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
managed this! for the capabilities loader, do you want a similar managemement? in my private example with credentials I am using the proxy url + the endpoint url of the wms capabilities in order to get the capabilities. but if you prefer I could add a preprocess url callback also for the capabilities loader
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there's no issue then I'd prefer not to add it since the user can construct it themselves before using the loader which otherwise isn't the case here. Is that right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think yes, it's fine 🚀
| preprocessURL = null, | ||
| } = options; | ||
| this.imageSource = null; | ||
| this.fetchOptions = fetchOptions; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the options on imageSource are really the ones being used here it may make most sense to add getters / setters for those options instead of storing them multiple places:
get fetchOptions() { return this.imageSource.fetchOptions; }
set fetchOptions( v ) { this.imageSource.fetchOptions = v; }There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, but in order to make fetchOptions still usable as a parameter in the constructor from the client (and so for usage in R3F) I had to set the imageSource fetchOptions in the WMSTilesOverlay using the options, otherwise I can't set from ImageOverlay the fetch Options before the imageSource is defined.
export class WMSTilesOverlay extends ImageOverlay {
constructor( options = {} ) {
super( options );
this.imageSource = new WMSImageSource( options );
if (options.fetchOptions) {
this.imageSource.fetchOptions = options.fetchOptions;
}
this.imageSource.fetchData = ( ...args ) => this.fetch( ...args );
}
... rest of the code
if this the way to go I would consider implementing the same logic for all other overlays
|
sorry to keep you waiting, I was sick these days. I just left one conversation open which I think is good point to discuss. |
According to this issue: #1308 I created this Pull Request.
The reason for the proxyUrl is that usually the front end can not directly communicate with external servers, causing CORS issues.
so a proxy is needed in order to manage the request server side, regardless of the need for custom fetch options.
the proxy is also used by the CesiumJS Library, here just a code example:
for the custom headers, the ImageOverlay class now takes also fetchOptions parameter and use them while fetching the imageSource.
just 2 things:
just for reference here an image of what I mean in the point 2.

if you notice something, don't hesitate to tell me!